Authentication improvements - OAuth Login #1511
Conversation
| @@ -31,22 +42,35 @@ const StyledButton = styled(Button)` | |||
| width: ${remSize(300)}; | |||
| `; | |||
|
|
|||
| function SocialAuthButton({ service }) { | |||
| function SocialAuthButton({ service, link, connected }) { | |||
There was a problem hiding this comment.
I spent a while thinking does link mean that this button's an href? What is it linking to? Maybe it's just pre morning-coffee.
Perhaps instead of link, just the "connected" prop is enough to change the label type. If connected is null then display the login label otherwise display the connect/connected labels?
There was a problem hiding this comment.
What I'm attempting to account for here is two different display options for the SocialAuthButton:
- One for the Login/Signup Page
- One for the Account Settings page, which uses the word "Link" rather than "Login". When the auth button is in "link" mode, it shows that a social account can either be "connected" or "not connected".
So maybe this just needs to be changed to use better named variables?
| * @callback [cb] - Optional error-first callback that passes User document | ||
| * @return {Promise<Object>} - Returns Promise fulfilled by User document | ||
| */ | ||
| userSchema.statics.findByEmailOrUsername = function findByEmailOrUsername(value, cb) { | ||
| userSchema.statics.findByEmailOrUsername = function findByEmailOrUsername(value, caseInsensitive, cb) { | ||
| const isEmail = value.indexOf('@') > -1; |
There was a problem hiding this comment.
We validate usernames not containing "@" chars in the client but I wonder whether this should also happen in the mongoose User model?
There was a problem hiding this comment.
Do you mean validating a username when saving a (new or existing) User document?
There was a problem hiding this comment.
Yes! Just to ensure these username rules are respected at the core of the app.
server/config/passport.js
Outdated
| user.name = profile.displayName; | ||
| user.verified = User.EmailConfirmation.Verified; | ||
| user.save(saveErr => done(null, user)); | ||
| let { username } = profile; |
There was a problem hiding this comment.
I'm a bit confused about when this flow will be triggered? If the user does not have an Editor account with the same email as their Github account?
There was a problem hiding this comment.
It's expected that this would trigger when a user is registering for the Editor with a GitHub account, i.e. they clicked "Login with GitHub" from the Signup Page.
This just reminded me that I should test if linking social accounts works properly if the email they registered with for the Editor is different from the email for their GItHub/Google Account.
|
I tested creating an account with a different email from the social account the user wants to link, and it will link the social account to the registered account matching the social's email, not the account the user explicitly tried to link to. So that's a bug 😄 |
Is there a way to write a test for this controller/passport functionality to help verify these different conditions? It's a bit tricky as it calls out to mongo quite a lot but it might be worth it? |
That's probably a good idea, I was testing this by having an open connection to the MongoDB (on mlab), then deleting the GitHub key and trying to re-link the account. |
|
Hey @oruburos! I'd like to add some Spanish translations in this PR, and if you could help out, that would be great! Specifically with the following phrases:
Thank you!! If you'd prefer I'd not ask you for translations in the future let me know, I'm happy to ask other Spanish-speaking folks in the community. |
|
Hi @catarak, here are my suggestions
|
|
@oruburos if you're up for it, I could use a few more translations:
Thank you for your help! |
|
@catarak Sure, here are my suggestions
|
|
@catarak, i am going through the branches, i think this one is ready to be deleted :) |
Fixes #915, Re: #1317
I discovered a bug with respect to email address duplications and OAuth login, so this PR addresses that, and cleans up the flow and language for linking social accounts from the Account Settings page.
I have verified that this pull request:
npm run lint)Fixes #123